Fix admin permission mismatch for design system deletion - #4695
Conversation
builder-io-integration
Bot
changed the title
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Here's a visual recap of what changed: Open the full interactive recap |
…to ai_main_fad3bc5b90f940509045
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🔴
Review Details
Code Review Summary
This incremental review rechecked PR #4695 at the latest head. The PR centralizes the admin-level management threshold for design systems, updates the list action to use the same rule, and adds coverage for admin/viewer authorization plus legacy Slides data rendering. The existing comments about saved-template references and wrong-type token values remain unresolved and were not reposted. This remains high risk because it changes authorization and destructive mutation behavior.
New Findings
- 🔴 High: Shared-admin deletion can clear
designSystemIdand update timestamps on every linked design, including designs the admin cannot edit. Access to a design system does not grant write access to all consumer designs. - 🔴 High: Authorization is checked before the deletion transaction, so a share revoked after
assertAccessbut before the mutation can still result in deletion by a now-revoked admin. - 🟡 Medium: Deleting another owner's default system does not promote a replacement, leaving that owner without an effective default in the Design app.
The role centralization itself is sound, and the delete/share cleanup remains transactional. The Slides implementation provides a useful comparison by handling default promotion and reporting inaccessible linked records rather than silently rewriting them.
🧪 Browser testing: Will run after this review (PR touches UI code)
| }), | ||
| run: async ({ id }) => { | ||
| await assertAccess("design-system", id, "owner"); | ||
| await assertAccess("design-system", id, DESIGN_SYSTEM_MANAGE_ROLE); |
There was a problem hiding this comment.
🔴 Revoked administrators can still complete a deletion
assertAccess runs before the deletion transaction. If an owner revokes the admin share after this check but before the transaction performs its updates/deletes, the now-revoked admin can still delete the system and unlink designs. Revalidate/lock the resource and access grant inside the mutation transaction or otherwise serialize share revocation with destructive deletion.
Additional Info
New issue identified in the latest incremental review.
There was a problem hiding this comment.
Respectfully disagree — not fixing this one, and it's not something introduced by this PR. assertAccess then act (no re-check inside the transaction) is the universal, documented pattern in this codebase: the sharing skill's own example is exactly assertAccess(...); // ...proceed, and I checked — all ~317 actions across every template that call assertAccess" use this same single-check-then-mutate shape, including ones that write inside a db.transaction(e.g.templates/assets/actions/duplicate-library.ts). The sibling Slides delete-design-system.ts this PR was calibrated against has the identical shape (assertAccessthendb.transaction`, no re-validation). Closing this race would mean wrapping every write action in the framework with a lock/re-check — a real architectural change, but far outside the scope of 'fix the admin/UI permission mismatch,' and inconsistent to apply to just this one action while leaving the other 300+ as-is. Happy to raise it as a framework-level follow-up if you'd like, but I'm not making an isolated change here that the rest of the codebase doesn't follow.
|
@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why |
SummaryReviewed all 5 PR feedback items skeptically, verified each against actual code, and replied on every thread: Fixed (4):
Disagreed (1):
All changes verified with new/updated regression tests (each confirmed to fail without the fix), full typecheck (clean except one pre-existing unrelated error), and all 71 repo guards passing. ⠀ |
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🔴
Review Details
Code Review Summary
This incremental review covers the latest expansion of the design-system delete flow. The PR now attempts to clean up linked designs and saved templates with per-resource access checks, promotes a replacement default, and normalizes wrong-typed Slides token values. Four previously reported issues were verified fixed and their stale threads were resolved: template cleanup, malformed token rendering, unauthorized cross-design rewrites, and missing default promotion. The revocation race remains unresolved and was not reposted.
New Findings
- 🔴 High: Default promotion uses the deleter’s active organization instead of the deleted system’s stored organization, which can promote an unrelated system in a cross-org shared-admin scenario.
- 🟡 Medium: Cleanup updates are not compare-and-set scoped to the deleted system, so a concurrently reassigned design/template can have its new link cleared.
- 🟡 Medium: Rejected
resolveAccessor unlink updates are swallowed byPromise.allSettled; the action returns success without reporting failed cleanup. - 🟡 Medium: Deletion performs unbounded per-resource access lookups, creating an N+1/full-row query pattern for heavily reused systems.
The access-aware cleanup direction is an improvement, but the post-transaction asynchronous cleanup needs stronger failure signaling and race-safe predicates before this destructive action is reliable.
🧪 Browser testing: Will run after this review (PR touches UI code)
| .delete(schema.designSystems) | ||
| .where(eq(schema.designSystems.id, id)); | ||
|
|
||
| if (access.resource.isDefault) { |
There was a problem hiding this comment.
🔴 Promote defaults in the deleted system's scope, not the deleter's active org
The replacement query uses getRequestOrgId() rather than access.resource.orgId. A shared admin can be active in org B while deleting a system owned in org A; this can leave org A without a replacement while promoting an unrelated system in B that the caller cannot access. Scope the replacement using the deleted resource's stored orgId, including its null/personal branch.
Additional Info
New issue identified by 1/3 review agents; high severity retained per review policy.
| if (!access || !canEditDesignRole(access.role)) { | ||
| return { id: designId, status: "skipped-no-access" }; | ||
| } | ||
| await getDb() |
There was a problem hiding this comment.
🟡 Do not clear a link that was reassigned during cleanup
The linked IDs are captured before the delete transaction, then cleanup runs later on separate database operations and updates by resource ID only. If a design or template is reassigned to another design system in that window, this action still sets its designSystemId to null and destroys the newer link. Include designSystemId = id in the update predicate and treat a zero-row compare-and-set as a stale assignment.
Additional Info
Identified independently by 3/3 code-review agents; severity normalized to medium by majority vote.
| // template we can't touch (missing access) is left dangling rather than | ||
| // retried here — both get-design-template and list-design-templates | ||
| // already mask a dangling designSystemId by resolving it back to null. | ||
| const [designResults, templateResults] = await Promise.all([ |
There was a problem hiding this comment.
🟡 Surface unlink failures instead of treating deletion as complete
Promise.allSettled captures rejected access checks and unlink updates, but the response only reports fulfilled skipped-no-access values. A rejected cleanup leaves a linked resource dangling while the action returns deleted: true with no retryable failure information. Preserve rejected IDs in explicit failure fields or fail with an explicit partial-cleanup result.
Additional Info
Identified independently by 3/3 code-review agents.
| // template we can't touch (missing access) is left dangling rather than | ||
| // retried here — both get-design-template and list-design-templates | ||
| // already mask a dangling designSystemId by resolving it back to null. | ||
| const [designResults, templateResults] = await Promise.all([ |
There was a problem hiding this comment.
🟡 Bound per-resource access lookups during deletion
The action starts one resolveAccess call per linked design and template concurrently, and each lookup can load the full resource plus share rows. A widely reused system therefore creates an unbounded N+1 query burst during deletion, risking connection-pool and request-memory pressure. Use a scoped bulk access query or bounded concurrency with an access-only projection.
Additional Info
Identified by 1/3 review agents; included because the unbounded fan-out is directly introduced by this deletion path.
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Code Review Summary
This incremental review covers the expanded deletion implementation in PR #4695. The update now limits unlinking to designs/templates the caller can edit, promotes a replacement default, reports skipped resources, and hardens Slides token merging against wrong runtime types. The core authorization alignment remains sound, and targeted action/UI-data tests were reported passing. This remains high risk because the action deletes shared resources and now returns cleanup metadata.
New Findings
- 🟡 Medium: The action returns IDs of inaccessible linked designs/templates in
designsSkippedForAccessandtemplatesSkippedForAccess, allowing a shared design-system admin to enumerate private resource IDs. - 🟡 Medium: Default promotion relies on
access.resource.isDefaultcaptured before the transaction. Concurrent deletion/promotion can make that value stale and leave the owner with no default system.
The previously reported issues about revocation races, deleted-system scope, reassigned links, swallowed unlink failures, and unbounded access lookups remain unresolved and were not reposted. The earlier malformed-token issue appears addressed by the new runtime-type fallback test and implementation, but its prior review comment was not cleared because it is not included in the currently listed unresolved comments.
🧪 Browser testing: Will run after this review (PR touches UI code)
| return { | ||
| id, | ||
| deleted: true, | ||
| ...(designsSkippedForAccess.length > 0 |
There was a problem hiding this comment.
🟡 Deletion response exposes IDs of inaccessible linked resources
The action selects every linked design/template, then returns inaccessible resource IDs in designsSkippedForAccess and templatesSkippedForAccess. A user with admin access only to the shared design system can enumerate private design/template IDs owned by others. Return aggregate counts or only identifiers the caller is authorized to read.
Additional Info
New issue identified in one focused review; no existing comment covers this response-data exposure.
| .delete(schema.designSystems) | ||
| .where(eq(schema.designSystems.id, id)); | ||
|
|
||
| if (access.resource.isDefault) { |
There was a problem hiding this comment.
🟡 Concurrent deletes can leave an owner with no default system
The promotion decision uses access.resource.isDefault captured before the transaction. In a concurrent sequence, another delete can promote this system after that access result is read, and this delete can then remove it while skipping the promotion branch because the value was stale. Re-read/lock the current row and owner scope inside the transaction before deciding whether to promote, and add a concurrent-delete regression test.
Additional Info
New issue identified in one focused review; distinct from the existing comment about using the deleter's org scope.
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Code Review Summary
This incremental review evaluated the expanded deletion cleanup at the latest PR head. The implementation now separates design-system authorization from access to linked designs/templates, reports skipped resources, promotes a replacement default, and normalizes wrong-typed Slides token leaves. The seven previously open deletion comments remain applicable and were not reposted. This remains high risk because it changes authorization, deletion ordering, and cross-resource cleanup.
New Findings
- 🟡 Medium: Linked design/template discovery performs full-table scans because the new
design_system_idlookups have no supporting indexes in the Design schema/migrations. This adds a request-path performance regression that scales with all workspace resources. - 🟡 Medium: Linked-resource IDs are snapshotted before deletion. A concurrent writer that passed its design-system access check before the delete can commit a new reference after the snapshot and after the system row is removed, leaving a dangling link that cleanup never visits. This is distinct from the existing open comment about clearing a reassigned link during cleanup.
The access-boundary changes and malformed-token normalization are directionally sound, and focused action/hook tests pass under their workspace configurations. I did not post the alias-resolution observation because it is dependent on invoking the test from the wrong repository-level configuration; the Slides workspace runner passes it.
🧪 Browser testing: Will run after this review (PR touches UI code)
| .update(schema.designs) | ||
| .set({ designSystemId: null, updatedAt: new Date().toISOString() }) | ||
| .where(eq(schema.designs.designSystemId, id)); | ||
| const [linkedDesignIds, linkedTemplateIds] = await Promise.all([ |
There was a problem hiding this comment.
🟡 Cleanup misses links attached after the pre-delete snapshot
The linked design/template IDs are read before the deletion transaction. A writer that passed its design-system access check before this delete can still commit a new designSystemId reference after the snapshot and after the system row is removed, so that link is never visited by cleanup and remains dangling. Serialize deletion with link writes or add a post-delete conditional cleanup that covers links created during the race.
Additional Info
New issue identified independently during the latest incremental review; distinct from the existing reassigned-link cleanup comment.
| const [linkedDesignIds, linkedTemplateIds] = await Promise.all([ | ||
| db | ||
| .select({ id: schema.designs.id }) | ||
| .from(schema.designs) | ||
| .where(eq(schema.designs.designSystemId, id)) | ||
| .then((rows) => rows.map((row) => row.id)), | ||
| db | ||
| .select({ id: schema.designTemplates.id }) | ||
| .from(schema.designTemplates) | ||
| .where(eq(schema.designTemplates.designSystemId, id)) |
There was a problem hiding this comment.
🟡 Linked-resource discovery performs full-table scans
The new pre-delete lookups filter designs and designTemplates by design_system_id, but the Design schema/migrations provide no supporting indexes for these columns. Every deletion now scans both resource tables, including systems with no links, so latency grows with the entire workspace. Add additive indexes for both link columns.
Additional Info
New issue identified during the latest incremental review; verified against the Design schema/migration definitions.
|
design preview: Open preview Built by GitHub Actions from |
|
slides preview: Open preview Built by GitHub Actions from |
|
clips preview: Open preview Built by GitHub Actions from |
There was a problem hiding this comment.
Builder reviewed your changes — no new findings
Review Details
Code Review Summary
This incremental review rechecked the latest PR head with randomized file ordering and focused on authorization, destructive cleanup, concurrency, default promotion, and test behavior. The diff is unchanged in the substantive deletion paths from the previous review cycle. The nine existing deletion comments remain unresolved and were not reposted. No additional high-confidence defects were found beyond those open issues. The malformed-token handling remains covered by runtime type normalization and focused tests. This remains high risk because the PR changes authorization and cross-resource deletion behavior.
Configured focused tests reported by the agents passed for the changed Design and Slides suites. One agent reproduced a repository-root Vitest alias failure for the Slides page-data test, while the Slides workspace runner passes; this was treated as a test-runner context issue rather than a new PR defect.
🧪 Browser testing: Will run after this review (PR touches UI code)
3 participants
Admins couldn't delete design systems despite other applications (such as slides) supporting admins deleting design systems.
Also fixed a related bug with slides
To clone this PR locally use the Github CLI with command
gh pr checkout 4695You can tag me at @BuilderIO for anything you want me to fix or change